[AOT] Added publishAOT test app to ensure OpenTelemetry SDK is AOT safe. #4392
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4392 +/- ##
==========================================
+ Coverage 84.73% 84.79% +0.05%
==========================================
Files 300 300
Lines 12064 12064
==========================================
+ Hits 10223 10230 +7
+ Misses 1841 1834 -7 |
Yun-Ting
changed the title
|
Hi eerhardt, could you help in reviewing this PR? Thank you. |
| /// | ||
| /// error IL2091: 'T' generic argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicConstructors'. | ||
| /// </summary> | ||
| [Fact] |
There was a problem hiding this comment.
Curious - would it make sense to have the AOT test build as a separate CI job instead of using unit test here? (easier to turn on/off, can run in parallel with existing unit test pipelines, etc.)
There was a problem hiding this comment.
I'm thinking while we are still developing, i.e. the warnings are > 0, we keep this as a unit test so that it's easier to capture the expected number of warnings and run the test locally.
Once we fixed/suppressed all the warnings, we can bring it to be a CI to get the above mentioning benefits.
There was a problem hiding this comment.
that it's easier to capture the expected number of warnings and run the test locally.
You should be able to run the CI check locally as well.
test/OpenTelemetry.AotCompatibility.TestApp/OpenTelemetry.AotCompatibility.TestApp.csproj
Show resolved
Hide resolved
test/OpenTelemetry.AotCompatibility.TestApp/OpenTelemetry.AotCompatibility.TestApp.csproj
Outdated
Show resolved
Hide resolved
|
|
||
| Assert.True(process.WaitForExit(milliseconds: 180_000), "dotnet publish command timed out after 180 seconds."); | ||
|
|
||
| Assert.True(process.ExitCode == 0, "Publishing the AotCompatibility app failed. See test output for more details."); |
There was a problem hiding this comment.
You may want to consider a way to baseline the existing warnings to start, and ensure new warnings aren't introduced as new features are added. I'm not sure the best way to do this, but maybe @vitek-karas @agocke or @sbomer knows of a good way?
In dotnet/sdk, we use the following method to verify the warnings (see the callers above this method):
There was a problem hiding this comment.
Other than baselining the output of the build I can't think of another way for NativeAOT right now.
ILLink supports suppressing warnings via XML files, which would be the best solution here, unfortunately that functionality isn't implemented in NativeAOT (it's on the list, but I don't know when it's going to happen).
NativeAOT does support suppressing warnings in code via assembly-level attributes, but those would have to go onto the assembly which produces the warnings, which is not useful to this case.
Yun-Ting
force-pushed
the
yunl/enableAOT_2
branch
from
8bb406c
to
09bcb4c
Compare
test/OpenTelemetry.AotCompatibility.Tests/OpenTelemetry.AotCompatibility.Tests.csproj
Outdated
Show resolved
Hide resolved
| var process = new Process | ||
| { | ||
| // set '-nodereuse:false /p:UseSharedCompilation=false' so the MSBuild and Roslyn server processes don't hang around, which may hang the test in CI | ||
| StartInfo = new ProcessStartInfo("dotnet", $"publish {testAppProject} --self-contained -nodereuse:false /p:UseSharedCompilation=false") |
There was a problem hiding this comment.
these can be achieved in the CI tasks itself
There was a problem hiding this comment.
Thank you for the info, Cijo : )
Please check out this comment: #4392 (comment)
Changes
Towards #3429
Alternative approach of #4370 to get the list of AOT warnings.
Added
OpenTelemetry.AotCompatibility.TestAppproject that references all the assemblies you want to analyze for AOT / trimming that targets net7.0. And set PublishAot=true in this project. Following approach inhttps://learn.microsoft.com/en-us/dotnet/core/deploying/trimming/prepare-libraries-for-trimming#show-all-warnings-with-sample-application
to get the AOT warnings from the dependent packages. Specifying
<TrimmerRootAssembly Include="YourLibraryName" />in an tag ensures that every part of the library is analyzed. It tells the trimmer that this assembly is a "root" which means the trimmer will analyze the assembly as if everything will be used, and traverses all possible code paths that originate from that assembly.Added a test
OpenTelemetry.AotCompatibility.Teststhat callsdotnet publishon the above Exe.EnsureAotCompatibilitywill be update to assert there are no warnings emitted when all the warnings are addressed.Current publish test logs:
publish_testApp.log
Merge requirement checklist